Skip to content

Shared + map#138

Merged
stephencelis merged 3 commits intopointfreeco:mainfrom
arnauddorgans:feature/shared-map
Apr 3, 2025
Merged

Shared + map#138
stephencelis merged 3 commits intopointfreeco:mainfrom
arnauddorgans:feature/shared-map

Conversation

@arnauddorgans
Copy link
Contributor

Adding a map method to both Shared and SharedReader

Motivation

Avoid sharing parent feature logic with a child.

E.g.

// Feature
@Shared var selectedID: ID 
var children: IdentifiedArrayOf<Child>

// Child
@SharedReader var isSelected: Bool 

// Child Initial State
.init(isSelected: $selectedID.map { $0 == id })

Note

It could also be cool to add a writable map method, but I don't see the use case today

@mbrandonw
Copy link
Member

Hi @arnauddorgans, thanks for exploring this!

However, it should not be necessary. Shared and SharedReader already support a mapping-like operation via dynamic member lookup, much how Binding works. So, you can just move your logic to a property or subscript and it should just work:

extension Child.ID {
  subscript(equals other: Self) -> Bool { self == other }
}

$selectedID[equals: id]  // SharedReader<Bool>

Can you give that a shot and let us know how it works out?

@arnauddorgans
Copy link
Contributor Author

arnauddorgans commented Mar 24, 2025

@mbrandonw hey, yes it works, today I have this extension in my app

public extension Equatable {
    subscript(isEqualTo other: Self) -> Bool {
        self == other
    }
}

But it feels so unnatural to write a subscript to be able to use it with the library
I think it would be more comfortable to handle map methods, like Sequence, AsyncSequence or Publisher

Plus, we can't extend Any or Sendable so we can't share custom Shared maps across types

@stephencelis
Copy link
Member

stephencelis commented Mar 25, 2025

@arnauddorgans One wrinkle is that the @Shared property wrapper's underlying reference's identity depends on the source shared key, which is a unique, hashable value, and then any transformations/projections along the way are also hashable, since they rely on hashable key paths.

This means that these 2 shared references are equal:

@Shared(.key) var value1
@Shared(.key) var value2

$value1.prop == $value2.prop  // true

But because the transform function above is not hashable, the implementation uses the object identity, instead, which will not be shared:

https://github.com/pointfreeco/swift-sharing/pull/138/files#diff-c5b71fccf18418e74ad127f0ae15e15ae16773156494acece9fa6d296edb0da0R672-R673

This means that mapped references will never be equal:

@Shared(.key) var value1
@Shared(.key) var value2

$value1.map { $0.prop } == $value2.map { $0.prop }  // true

It's possible this isn't a big deal with shared readers, but at the same time it's a subtle enough difference from all the existing shared references that it should be carefully considered. I wonder if you can come up with some more edge cases to test and explore considering this mismatch?

One other thing that would be good is to get coverage on some actual observation, since that's probably the entire point of such a shared reader. Both tests on observe firing when we expect it to, as well as tests on publisher emitting with the transformed value would be good to have.

@arnauddorgans
Copy link
Contributor Author

@stephencelis I don't think the object comparison is a big deal, it is already done for _BoxReference and _PersistentReference

Plus, the Hashable constraint is annoying because it only constrains us to use Hashable parameters.

Screenshot 2025-03-26 at 03 56 30

I'll try to write some additional tests when I have the time 👍

@stephencelis
Copy link
Member

@arnauddorgans The difference is that _Box and _PersistentReference are the root references and thus there is only a single one shared amongst all projected children.

In the case of map, every time a map happens a new object is created, even though they may reference the same underlying data. Again, this may be OK, but I haven't yet fully wrapped my head around the difference yet to see if there's a problem 😄

@arnauddorgans
Copy link
Contributor Author

@stephencelis in my last commit, I deleted the Equatable conformance when the reference is not writable.
It wasn't needed, so the reference check that I wrote has been deleted as map is only available for SharedReader

This way I think that everything is working as all the existing shared references

So it is totally legit that

@Shared(value: 0) var base: Int
@SharedReader var lhs: Int
@SharedReader var rhs: Int
_lhs = $base.map { $0 * 2 }
_rhs = $base.map { $0 * 3 }
#expect(lhs == rhs)
#expect($lhs == $rhs)
    
$base.withLock { $0 += 1 }
#expect(lhs != rhs)
#expect($lhs != $rhs)

@stephencelis
Copy link
Member

While we think on this, @jshier I believe you were asking for something like this?

@jshier
Copy link

jshier commented Apr 1, 2025

Yes, I needed a simple way to create a SharedReader from a parent Shared value using a query to a parent opaque blob, whether on the fly or through a protocol abstraction. Only way I got it to work was to create a computed property in an extension on the parent type, which made it too unergonomic to use for dozens of different results across the codebase. I abandoned the approach for that reason and we just use the parent blob, which is awkward for tests but otherwise works fine.

That particular use case could also use a better solution for exposing custom APIs out Shared values, or their wrappers.

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnauddorgans Chatted through this again today and I think we're down to include this functionality. Just a couple changes and I think we'll be good to go! Let us know what you think 😄

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnauddorgans Updated reference looks good! Want to make a final change of renaming map to reader and we can get this merged?

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last bike shed!

@stephencelis stephencelis merged commit 5a2a704 into pointfreeco:main Apr 3, 2025
6 checks passed
mhayes853 pushed a commit to mhayes853/swift-sharing that referenced this pull request Aug 12, 2025
* map

* naming

* Apply suggestions from code review

---------

Co-authored-by: Stephen Celis <stephen.celis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants